❮

Memory leak fix in psql

  • Jump to comment-1
    tanghy.fnst@fujitsu.com2022-07-19T09:02:07+00:00
    Hi I think there is a newly introduced memory leak in your patch d2d3547. Try to fix it in the attached patch. Kindly to have a check. Regards, Tang
    • Jump to comment-1
      mark.dilger@enterprisedb.com2022-07-19T16:43:21+00:00
      > On Jul 19, 2022, at 2:02 AM, tanghy.fnst@fujitsu.com wrote: > > I think there is a newly introduced memory leak in your patch d2d3547. I agree. Thanks for noticing, and for the patch! > Try to fix it in the attached patch. > Kindly to have a check. This looks ok, but comments down-thread seem reasonable, so I suspect a new patch will be needed. Would you like to author it, or would you prefer that I, as the guilty party, do so? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
      • Jump to comment-1
        michael@paquier.xyz2022-07-20T00:01:13+00:00
        On Tue, Jul 19, 2022 at 09:43:21AM -0700, Mark Dilger wrote: > This looks ok, but comments down-thread seem reasonable, so I > suspect a new patch will be needed. Would you like to author it, or > would you prefer that I, as the guilty party, do so? If any of you could update the patch, that would be great. Thanks! -- Michael
    • Jump to comment-1
      japinli@hotmail.com2022-07-19T10:41:13+00:00
      On Tue, 19 Jul 2022 at 17:02, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > Hi > > I think there is a newly introduced memory leak in your patch d2d3547. > Try to fix it in the attached patch. > Kindly to have a check. > Yeah, it leaks, and the patch can fix it. After looking around, I found psql/describe.c also has some memory leaks, attached a patch to fix these leaks. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
      • Jump to comment-1
        tanghy.fnst@fujitsu.com2022-07-20T03:14:35+00:00
        On Tuesday, July 19, 2022 7:41 PM, Japin Li <japinli@hotmail.com> wrote: > After looking around, I found psql/describe.c also has some memory > leaks, > attached a patch to fix these leaks. Thanks for your check and improvement. Your fix LGTM so please allow me to merge it in the attached patch. Based on your rebased version, now this new patch version is V3. Regards, Tang
        • Jump to comment-1
          michael@paquier.xyz2022-07-20T03:51:32+00:00
          On Wed, Jul 20, 2022 at 03:14:35AM +0000, tanghy.fnst@fujitsu.com wrote: > Your fix LGTM so please allow me to merge it in the attached patch. > Based on your rebased version, now this new patch version is V3. What about the argument of upthread where we could use a goto in functions where there are multiple pattern validation checks? Per se v4 attached. -- Michael
          • Jump to comment-1
            tanghy.fnst@fujitsu.com2022-07-20T06:21:36+00:00
            On Wednesday, July 20, 2022 12:52 PM, Michael Paquier <michael@paquier.xyz> wrote: > What about the argument of upthread where we could use a goto in > functions where there are multiple pattern validation checks? Per se > v4 attached. Thanks for your kindly remind and modification. I checked v4 patch, it looks good but I think there can be some minor improvement. So I deleted some redundant braces around "goto error_return; ". Also added an error handle section in validateSQLNamePattern. Kindly to have a check at the attached v5 patch. Regards, Tang
            • Jump to comment-1
              japinli@hotmail.com2022-07-20T08:13:11+00:00
              On Wed, 20 Jul 2022 at 14:21, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > On Wednesday, July 20, 2022 12:52 PM, Michael Paquier <michael@paquier.xyz> wrote: >> What about the argument of upthread where we could use a goto in >> functions where there are multiple pattern validation checks? Per se >> v4 attached. > > Thanks for your kindly remind and modification. > I checked v4 patch, it looks good but I think there can be some minor improvement. > So I deleted some redundant braces around "goto error_return; ". > Also added an error handle section in validateSQLNamePattern. > > Kindly to have a check at the attached v5 patch. > > Regards, > Tang Thanks for updating the patch. It looks good. However, it cannot be applied on 14 stable. The attached patches are for 10-14. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
              • Jump to comment-1
                tgl@sss.pgh.pa.us2022-07-20T13:54:32+00:00
                Japin Li <japinli@hotmail.com> writes: > Thanks for updating the patch. It looks good. However, it cannot be > applied on 14 stable. The attached patches are for 10-14. While I think this is good cleanup, I'm doubtful that any of these leaks are probable enough to be worth back-patching into stable branches. The risk of breaking something should not be neglected. regards, tom lane
                • Jump to comment-1
                  japinli@hotmail.com2022-07-21T01:10:43+00:00
                  On Wed, 20 Jul 2022 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Japin Li <japinli@hotmail.com> writes: >> Thanks for updating the patch. It looks good. However, it cannot be >> applied on 14 stable. The attached patches are for 10-14. > > While I think this is good cleanup, I'm doubtful that any of these > leaks are probable enough to be worth back-patching into stable > branches. The risk of breaking something should not be neglected. > Yeah, we should take care of the backpatch risk. However, I think it makes sense to backpatch. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
                  • Jump to comment-1
                    michael@paquier.xyz2022-07-21T01:48:13+00:00
                    On Thu, Jul 21, 2022 at 09:10:43AM +0800, Japin Li wrote: > Yeah, we should take care of the backpatch risk. However, I think > it makes sense to backpatch. We are talking about 256 bytes being leaked in each loop when a validation pattern or when a query fails, so I don't see a strong argument in manipulating 10~14 more than necessary for this amount of memory. The contents of describe.c are the same for v15 though, and we are still in beta on REL_15_STABLE, so I have applied the patch down to v15, adding what Alvaro has sent on top of the rest. -- Michael
                    • Jump to comment-1
                      japinli@hotmail.com2022-07-21T06:02:49+00:00
                      On Thu, 21 Jul 2022 at 09:48, Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Jul 21, 2022 at 09:10:43AM +0800, Japin Li wrote: >> Yeah, we should take care of the backpatch risk. However, I think >> it makes sense to backpatch. > > We are talking about 256 bytes being leaked in each loop when a > validation pattern or when a query fails, so I don't see a strong > argument in manipulating 10~14 more than necessary for this amount of > memory. The contents of describe.c are the same for v15 though, and > we are still in beta on REL_15_STABLE, so I have applied the patch > down to v15, adding what Alvaro has sent on top of the rest. Thanks for the explanation! IMO, we could ignore v10-13 branches, however, we should backpatch to v14 which also uses the validateSQLNamePattern() function leading to a memory leak. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
                      • Jump to comment-1
                        alvherre@alvh.no-ip.org2022-07-21T06:23:45+00:00
                        On 2022-Jul-21, Japin Li wrote: > On Thu, 21 Jul 2022 at 09:48, Michael Paquier <michael@paquier.xyz> wrote: > > We are talking about 256 bytes being leaked in each loop when a > > validation pattern or when a query fails, so I don't see a strong > > argument in manipulating 10~14 more than necessary for this amount of > > memory. The contents of describe.c are the same for v15 though, and > > we are still in beta on REL_15_STABLE, so I have applied the patch > > down to v15, adding what Alvaro has sent on top of the rest. > > Thanks for the explanation! IMO, we could ignore v10-13 branches, however, > we should backpatch to v14 which also uses the validateSQLNamePattern() > function leading to a memory leak. I'd agree in principle, but in practice the commit from 15 does not apply cleanly on 14 -- there's a ton of conflicts. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Here's a general engineering tip: if the non-fun part is too complex for you to figure out, that might indicate the fun part is too ambitious." (John Naylor) https://postgr.es/m/CAFBsxsG4OWHBbSDM%3DsSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g%40mail.gmail.com
                        • Jump to comment-1
                          japinli@hotmail.com2022-07-21T06:43:03+00:00
                          On Thu, 21 Jul 2022 at 14:23, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2022-Jul-21, Japin Li wrote: > >> On Thu, 21 Jul 2022 at 09:48, Michael Paquier <michael@paquier.xyz> wrote: > >> > We are talking about 256 bytes being leaked in each loop when a >> > validation pattern or when a query fails, so I don't see a strong >> > argument in manipulating 10~14 more than necessary for this amount of >> > memory. The contents of describe.c are the same for v15 though, and >> > we are still in beta on REL_15_STABLE, so I have applied the patch >> > down to v15, adding what Alvaro has sent on top of the rest. >> >> Thanks for the explanation! IMO, we could ignore v10-13 branches, however, >> we should backpatch to v14 which also uses the validateSQLNamePattern() >> function leading to a memory leak. > > I'd agree in principle, but in practice the commit from 15 does not apply > cleanly on 14 -- there's a ton of conflicts. I attached a patch for v14 [1] based on master, if you want to apply it, please consider reviewing it. [1] https://www.postgresql.org/message-id/MEYP282MB166974D3883A1A6E25B5FDAEB68E9%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
                          • Jump to comment-1
                            michael@paquier.xyz2022-07-22T01:02:54+00:00
                            On Thu, Jul 21, 2022 at 02:43:03PM +0800, Japin Li wrote: > I attached a patch for v14 [1] based on master, if you want to apply it, > please consider reviewing it. We are talking about a few hundred bytes leaked each time, so this does not worry me much in the older branches, honestly. -- Michael
          • Jump to comment-1
            zhjwpku@gmail.com2022-07-20T04:51:24+00:00
            -1 Though the patch looks good, I myself think the patch should be edited and submitted by Tang It's easy to attach a fixed patch based on the comments of the thread, but coins should be given to Tang since he is the first one to find the mem leak. No offense, but that's what I think how open source works ;) On Wed, Jul 20, 2022 at 11:51 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 20, 2022 at 03:14:35AM +0000, tanghy.fnst@fujitsu.com wrote: > > Your fix LGTM so please allow me to merge it in the attached patch. > > Based on your rebased version, now this new patch version is V3. > > What about the argument of upthread where we could use a goto in > functions where there are multiple pattern validation checks? Per se > v4 attached. > -- > Michael -- Regards Junwang Zhao
            • Jump to comment-1
              michael@paquier.xyz2022-07-20T05:14:21+00:00
              On Wed, Jul 20, 2022 at 12:51:24PM +0800, Junwang Zhao wrote: > Though the patch looks good, I myself think the patch should be edited > and submitted by Tang > It's easy to attach a fixed patch based on the comments of the thread, > but coins should be > given to Tang since he is the first one to find the mem leak. Please note that I sometimes edit slightly patches that I finish to merge into the tree, where the author listed in the commit log is the same as the original while I usually don't list mine. Credit goes where it should, and Tang is the one here who authored this patch. -- Michael
              • Jump to comment-1
                zhjwpku@gmail.com2022-07-20T05:56:38+00:00
                Got it, thanks! On Wed, Jul 20, 2022 at 1:14 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 20, 2022 at 12:51:24PM +0800, Junwang Zhao wrote: > > Though the patch looks good, I myself think the patch should be edited > > and submitted by Tang > > It's easy to attach a fixed patch based on the comments of the thread, > > but coins should be > > given to Tang since he is the first one to find the mem leak. > > Please note that I sometimes edit slightly patches that I finish to > merge into the tree, where the author listed in the commit log is the > same as the original while I usually don't list mine. Credit goes > where it should, and Tang is the one here who authored this patch. > -- > Michael -- Regards Junwang Zhao
                • Jump to comment-1
                  tanghy.fnst@fujitsu.com2022-07-20T07:03:47+00:00
                  > On Wed, Jul 20, 2022 at 12:51:24PM +0800, Junwang Zhao wrote: > > Though the patch looks good, I myself think the patch should be edited > > and submitted by Tang > > It's easy to attach a fixed patch based on the comments of the thread, > > but coins should be > > given to Tang since he is the first one to find the mem leak. Hello, Zhao Thanks for your check at this patch. I appreciate your kindly comment but there may be a misunderstanding here. As Michael explained, committers in Postgres will review carefully and help to improve contributors' patches. When the patch is finally committed by one committer, from what I can see, he or she will try to make sure the credit goes with everyone who contributed to the committed patch(such as bug reporter, patch author, tester, reviewer etc.). Also, developers and reviewers will try to help improving our proposed patch by rebasing it or adding an on-top patch(like Japin Li did in v2). These will make the patch better and to be committed ASAP. Good to see you at Postgres community. Regards, Tang
                  • Jump to comment-1
                    zhjwpku@gmail.com2022-07-20T08:03:02+00:00
                    Thanks for your explanation, this time I know how it works, thanks ;) On Wed, Jul 20, 2022 at 3:04 PM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > > On Wed, Jul 20, 2022 at 12:51:24PM +0800, Junwang Zhao wrote: > > > Though the patch looks good, I myself think the patch should be edited > > > and submitted by Tang > > > It's easy to attach a fixed patch based on the comments of the thread, > > > but coins should be > > > given to Tang since he is the first one to find the mem leak. > > Hello, Zhao > > Thanks for your check at this patch. > > I appreciate your kindly comment but there may be a misunderstanding here. > As Michael explained, committers in Postgres will review carefully and > help to improve contributors' patches. When the patch is finally committed > by one committer, from what I can see, he or she will try to make sure the > credit goes with everyone who contributed to the committed patch(such as > bug reporter, patch author, tester, reviewer etc.). > > Also, developers and reviewers will try to help improving our proposed patch > by rebasing it or adding an on-top patch(like Japin Li did in v2). > These will make the patch better and to be committed ASAP. > > Good to see you at Postgres community. > > Regards, > Tang -- Regards Junwang Zhao
          • Jump to comment-1
            alvherre@alvh.no-ip.org2022-07-20T08:05:47+00:00
            More on the same tune. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "This is what I like so much about PostgreSQL. Most of the surprises are of the "oh wow! That's cool" Not the "oh shit!" kind. :)" Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php
            • Jump to comment-1
              michael@paquier.xyz2022-07-21T01:02:12+00:00
              On Wed, Jul 20, 2022 at 10:05:47AM +0200, Alvaro Herrera wrote: > More on the same tune. Thanks. I have noticed that as well. I'll include all that in the set. -- Michael
          • Jump to comment-1
            japinli@hotmail.com2022-07-20T04:01:12+00:00
            On Wed, 20 Jul 2022 at 11:51, Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Jul 20, 2022 at 03:14:35AM +0000, tanghy.fnst@fujitsu.com wrote: >> Your fix LGTM so please allow me to merge it in the attached patch. >> Based on your rebased version, now this new patch version is V3. > > What about the argument of upthread where we could use a goto in > functions where there are multiple pattern validation checks? Per se > v4 attached. +1. LGTM. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
      • Jump to comment-1
        michael@paquier.xyz2022-07-19T12:32:07+00:00
        On Tue, Jul 19, 2022 at 06:41:13PM +0800, Japin Li wrote: > After looking around, I found psql/describe.c also has some memory leaks, > attached a patch to fix these leaks. Indeed. There are quite a bit of them, so let's fix all that. You have missed a couple of code paths in objectDescription(). -- Michael
        • Jump to comment-1
          japinli@hotmail.com2022-07-19T13:08:53+00:00
          On Tue, 19 Jul 2022 at 20:32, Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Jul 19, 2022 at 06:41:13PM +0800, Japin Li wrote: >> After looking around, I found psql/describe.c also has some memory leaks, >> attached a patch to fix these leaks. > > Indeed. There are quite a bit of them, so let's fix all that. You > have missed a couple of code paths in objectDescription(). Thanks for reviewing. Attached fix the memory leak in objectDescription(). Please consider v2 for further review. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
          • Jump to comment-1
            andres@anarazel.de2022-07-19T16:28:14+00:00
            Hi, On 2022-07-19 21:08:53 +0800, Japin Li wrote: > From b2bcc3a1bac67b8b414f2025607f8dd35e096289 Mon Sep 17 00:00:00 2001 > From: Japin Li <japinli@hotmail.com> > Date: Tue, 19 Jul 2022 18:27:25 +0800 > Subject: [PATCH v2 1/1] Fix the memory leak in psql describe > > --- > src/bin/psql/describe.c | 168 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 168 insertions(+) > > diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c > index 0ce38e4b4c..7a070a6cd0 100644 > --- a/src/bin/psql/describe.c > +++ b/src/bin/psql/describe.c > @@ -112,7 +112,10 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem) > "n.nspname", "p.proname", NULL, > "pg_catalog.pg_function_is_visible(p.oid)", > NULL, 3)) > + { > + termPQExpBuffer(&buf); > return false; > + } > > appendPQExpBufferStr(&buf, "ORDER BY 1, 2, 4;"); Adding copy over copy of this same block doesn't seem great. Can we instead add a helper for it or such? Greetings, Andres Freund
            • Jump to comment-1
              tgl@sss.pgh.pa.us2022-07-19T16:36:31+00:00
              Andres Freund <andres@anarazel.de> writes: > On 2022-07-19 21:08:53 +0800, Japin Li wrote: >> + { >> + termPQExpBuffer(&buf); >> return false; >> + } > Adding copy over copy of this same block doesn't seem great. Can we instead > add a helper for it or such? The usual style in these files is something like if (bad things happened) goto fail; ... fail: termPQExpBuffer(&buf); return false; Yeah, it's old school, but please let's not have a few functions that do it randomly differently from all their neighbors. regards, tom lane
              • Jump to comment-1
                michael@paquier.xyz2022-07-19T23:59:48+00:00
                On Tue, Jul 19, 2022 at 12:36:31PM -0400, Tom Lane wrote: > Yeah, it's old school, but please let's not have a few functions that > do it randomly differently from all their neighbors. True enough. And it is not like we should free the PQExpBuffer given by the caller in validateSQLNamePattern(). -- Michael